feat: add taxonomy api pagination - #69
Conversation
df959bf to
1d3d0e6
Compare
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
👍 Thank you for this quick fix @rpenido !
Couple of small nits, but this should be good to merge when they're addressed.
- I tested this by running the dev server
Logged in via admin, created a taxonomy, and visited the taxonomies list API to check that pagination works.
❯ python3 manage.py migrate ❯ python3 manage.py createsuperuser ❯ python3 manage.py runserver - I read through the code and checked the tests.
-
I checked for accessibility issuesN/A - Includes documentation
-
I made sure any change in configuration variables is reflected in the correspondingN/A
client'sconfiguration-securerepository.
3b089f5 to
7e5d7f4
Compare
7e5d7f4 to
dd1b2de
Compare
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
@bradenmacdonald or @ormsbee Could you give this a look when you get a chance? It should be a small change, which I'm hoping isn't controversial. (cf internal OpenCraft link: FAL-3450). |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
LGTM, just one change please.
| class TaggingPagination(PageNumberPagination): | ||
| page_size_query_param = "page_size" | ||
| page_size = 100 | ||
| max_page_size = 1000 |
There was a problem hiding this comment.
I believe you should just use edx_rest_framework_extensions.paginators.DefaultPagination instead.
The platform sets it as a global default for all APIs - maybe we can do so here too?
There was a problem hiding this comment.
Hi @bradenmacdonald!
I believe you should just use
edx_rest_framework_extensions.paginators.DefaultPaginationinstead.
In a discussion with @pomegranited open-craft#4 (comment), we talked that we shouldn't import edx-platform here. I think I misunderstood it and avoided importing other apps from edx in general. If this is not a problem, we should use this default pagination class.
The platform sets it as a global default for all APIs - maybe we can do so here too?
I think we can. Note that we have another rest_api in this app that will also be affected (not implemented, thou):
If this is fine, we can set a default for all APIs.
There was a problem hiding this comment.
Awesome, thanks for that recommendation @bradenmacdonald , I didn't know that existed.
@rpenido Yes please add this dependency and use edx_rest_framework_extensions.paginators.DefaultPagination as our default for this library instead of creating our own class here.
It's ok to just use it directly, and keep its page_size and max_page_size too -- we can adjust these later if we need them in the Tagging UI.
There was a problem hiding this comment.
@pomegranited I think you pinged the wrong person ;)
Thanks! Should I revert the style commit? |
I don't think that's what @bradenmacdonald meant by this.. it's ok to keep your style change. |
|
Yup, I just meant "please just make the one change about using the existing pagination class, otherwise I think this looks great" |
|
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
4 participants
This PR add pagination to the Taxonomy API